Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Defensive guards against invalid code points in preferences. #6094

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Defensive guards against invalid code points in preferences. #6094

merged 1 commit into from
Jul 4, 2023

Conversation

mbien
Copy link
Member

@mbien mbien commented Jun 19, 2023 •
edited
Loading

The code point '\u0000' breaks the Preferences storage as demonstrated in https://bugs.openjdk.org/browse/JDK-8068373, as preventive measure it can't be used in keys since https://bugs.openjdk.org/browse/JDK-8084823.

There are many reports of windows users running into seemingly unrelated issues, but closer investigation showed that they all had at some point "invalid code point" exceptions in the log.

this usually happens in a code pattern like this:

for (String k : pref.keys()) { // contains an invalid key
    ... = pref.get(key, null);  // throws IAE
}

It is not clear what causes the corruption (and why it seems to only occur on windows), this PR adds some defensive code to cover some common cases, logs it and makes sure no corrupted properties files are imported from old configs on first launch.

the jackpot rule:

$pref.keys() :: $pref instanceof java.util.prefs.Preferences;;

had 70 hits - this PR covers only common cases + adds the import filter.

org.apache.tools.ant.module.bridge.impl.BridgeImpl$PostRun
java.lang.IllegalArgumentException: Key contains code point U+0000
	at java.prefs/java.util.prefs.AbstractPreferences.get(AbstractPreferences.java:296)
	at org.apache.tools.ant.module.api.IntrospectedInfo$3.load(IntrospectedInfo.java:681)
	at org.apache.tools.ant.module.AntSettings.getCustomDefs(AntSettings.java:119)
	at org.apache.tools.ant.module.bridge.impl.BridgeImpl$PostRun.run(BridgeImpl.java:443)
	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1418)
	at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)

java.lang.IllegalArgumentException: Key contains code point U+0000
       at java.util.prefs.AbstractPreferences.get(AbstractPreferences.java:296)
       at java.util.prefs.AbstractPreferences.getBoolean(AbstractPreferences.java:531)
       at org.netbeans.core.windows.TopComponentTracker.load(TopComponentTracker.java:99)
       at org.netbeans.core.windows.PersistenceHandler.load(PersistenceHandler.java:126)
       at org.netbeans.core.windows.WindowSystemImpl.load(WindowSystemImpl.java:81)
       at org.netbeans.core.GuiRunLevel$InitWinSys.run(GuiRunLevel.java:229)

java.lang.IllegalArgumentException: Key contains code point U+0000
      at java.prefs/java.util.prefs.AbstractPreferences.get(AbstractPreferences.java:296)
      at org.netbeans.modules.maven.queries.MavenFileOwnerQueryImpl.registerCoordinates(MavenFileOwnerQueryImpl.java:153)
      at org.netbeans.modules.maven.ProjectOpenedHookImpl.registerWithSubmodules(ProjectOpenedHookImpl.java:431)
      at org.netbeans.modules.maven.ProjectOpenedHookImpl.projectOpened(ProjectOpenedHookImpl.java:138)
      at org.netbeans.spi.project.ui.ProjectOpenedHook$1.projectOpened(ProjectOpenedHook.java:60)
  [catch] at org.netbeans.modules.project.ui.OpenProjectList.notifyOpened(OpenProjectList.java:1273)
      at org.netbeans.modules.project.ui.OpenProjectList.doOpenProject(OpenProjectList.java:1354)
      at org.netbeans.modules.project.ui.OpenProjectList.open(OpenProjectList.java:798)
      at org.netbeans.modules.project.ui.OpenProjectList$6.run(OpenProjectList.java:650)
      at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1418)
      at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
      at org.openide.util.lookup.Lookups.executeWith(Lookups.java:278)
      at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2033)

closes #6076
closes #5147
closes #5633
closes #5634
https://bz.apache.org/netbeans/show_bug.cgi?id=271652
https://issues.apache.org/jira/browse/NETBEANS-3284

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Maven [ci] enable "build tools" tests Platform [ci] enable platform tests (platform/*) Ant [ci] enable "build tools" tests labels Jun 19, 2023
@mbien mbien added this to the NB19 milestone Jun 19, 2023
@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jun 19, 2023
@apache apache locked and limited conversation to collaborators Jun 19, 2023
@apache apache unlocked this conversation Jun 19, 2023
neilcsmith-net
neilcsmith-net approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@neilcsmith-net neilcsmith-net left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally a good move. Thanks for picking up!

On the protecting on import side -

@mbien
Copy link
Member Author

mbien commented Jun 19, 2023 •
edited
Loading

there are .properties files outside of config/Preferences. Are we 100% sure there is no valid reason for \u0000 in them? Or need to filter by preferences root?

I scanned my config folder which had NB configs reaching back to NB 5. And none of them had this code point in properties files. #6076 (comment)

does the import protection also need to be considered in .. (yes, I love we have two code paths with somewhat different implementations of similar things!)

oh boy. No I have not. Going to take a look.

OptionsExportModel isn't closing a single ZipFile instance...

@mbien
Copy link
Member Author

mbien commented Jun 19, 2023 •
edited
Loading

@neilcsmith-net good that you pointed me to OptionsExportModel, since it left several streams open, and all ZipFiles.

I never used the zip export feature before. Lets test this a bit before merging.

edit: tested config export/import and also config migration, everything worked fine I believe. Corrupted properties were also detected and successfully ignored.

forgot to squash, will force push

@mbien
Defensive guards against invalid code points in preferences.
a208c99 
Made sure that OptionsExportModel is closing streams and zip files.
@mbien mbien force-pushed the invalid-code-point branch from 76fdd1b to a208c99 Compare June 19, 2023 21:10
@mbien mbien mentioned this pull request Jun 28, 2023
Closed
matthiasblaesing
matthiasblaesing approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the \0 in properties, but the changes look sane and code is cleaner now.

@mbien
Copy link
Member Author

mbien commented Jul 4, 2023

thanks for the reviews and sorry for the delay.

I am also not sure about the null code point as property value, but https://bugs.openjdk.org/browse/JDK-8068373 for example describes it being illegal in XML files. Given that a) there was no such value in any property in my config backups and b) properties could end up in xml at some point which makes it probably safer to filter them out during config migration.

-> merging

@mbien mbien merged commit 57335c4 into apache:master Jul 4, 2023
@BruceKHaddon BruceKHaddon mentioned this pull request Jul 4, 2023
Closed
@pepness pepness mentioned this pull request Jul 18, 2023
Closed
@mbien mbien mentioned this pull request Jul 18, 2023
Merged
@mbien mbien mentioned this pull request Aug 21, 2023
Closed
@mbien mbien mentioned this pull request Apr 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasblaesing matthiasblaesing matthiasblaesing approved these changes

@neilcsmith-net neilcsmith-net neilcsmith-net approved these changes

@lkishalmi lkishalmi Awaiting requested review from lkishalmi

Assignees
No one assigned
Labels
Ant [ci] enable "build tools" tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Maven [ci] enable "build tools" tests Platform [ci] enable platform tests (platform/*)
Projects
None yet
Milestone
NB19
3 participants
@mbien @matthiasblaesing @neilcsmith-net